Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hover fix #10333

Merged
merged 7 commits into from
Nov 14, 2018
Merged

Hover fix #10333

merged 7 commits into from
Nov 14, 2018

Conversation

jobthomas
Copy link
Contributor

@jobthomas jobthomas commented Oct 4, 2018

Description

Fixes #10320

How has this been tested?

Tested on iOS 12 Safari - and Firefox Focus

Screenshots

Types of changes

Disabling :hover for mobile

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@tofumatt
Copy link
Member

tofumatt commented Oct 4, 2018

Any chance we could get before/after screen recording of this in action? That'd make review easier ❤️

@jobthomas
Copy link
Contributor Author

Before: https://cld.wthms.co/VbiBYh - slight colour change when putting fingers on items for scrolling

After: http://cld.wthms.co/LhbM5v - I deliberately put my fingers above the menu items while scrolling and no more colour changes.

@tofumatt tofumatt self-requested a review October 8, 2018 10:41
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot; thought I approved this but I must've got distracted by another tab or a shiny object 😅

Thanks for the PR and thanks for the screenshots; they really helped!

I tweaked your comment to point to the PR and this looks good. I'll merge it once Travis is green 😄

@jobthomas
Copy link
Contributor Author

Seems like Travis isn't happy ;) Thanks for the update @tofumatt

@tofumatt
Copy link
Member

Travis was buggy with this PR and the failures are complaining about PHP files not in this change 🤷‍♂️

Just gonna merge because this works for me locally...

@tofumatt tofumatt merged commit 721dbab into WordPress:master Nov 14, 2018
@tofumatt tofumatt added this to the 4.4 milestone Nov 14, 2018
@shaunroselt
Copy link

shaunroselt commented Nov 19, 2018

So I haven't tested this yet, but it sounds sad when reading this.

I actually use my phone (Samsung Galaxy S9+) with Samsung DeX for all of my development work. I hope hovering will still work on my phone. I don't want this disabled at all. It will be awful if :hover is disabled for mobile.

@jobthomas
Copy link
Contributor Author

@shaunroselt could you share a specific example of this? With most phones, it's required to actually click something in order to open up a section, not hover over (you can't hover with a finger).

That said, the only change it that it doesn't show up as a different colour on mobile.

@shaunroselt
Copy link

@shaunroselt could you share a specific example of this? With most phones, it's required to a

@jobthomas You can however hover with a mouse. I have an external monitor, keyboard and mouse plugged into my phone and then I use it like this. So I actually use the :hover on mobile with my mouse.

I plan on slowly upgrading all of my old websites to WordPress 5 and Gutenberg when it comes out as well as do all my future websites in WordPress 5 and Gutenberg. I definitely want hover to keep working on my phone. I have a mouse connected to my phone 70% of the time. So I don't really use a finger at all, but I do use a mouse. I have a Logitech G502 Mouse at home and a Cooler Master MS120 Mouse at work. I use them both with my phone.

@jobthomas
Copy link
Contributor Author

Just as reference, the $break-medium is set to 782px. Based on what I can read, the Galaxy S9 is a higher resolution than that, so that would mean this pull request doesn't affect your device. The external monitor should also not be affected by this at all.

All this does is not change the colours of the hover option if the screen is less wide than 782px.

@shaunroselt
Copy link

Just as reference, the $break-medium is set to 782px. Based on what I can read, the Galaxy S9 is a higher resolution than that, so that would mean this pull request doesn't affect your device. The external monitor should also not be affected by this at all.

All this does is not change the colours of the hover option if the screen is less wide than 782px.

@jobthomas Okay that sounds better. Thanks. I have a 1920x1080 monitor at home and work for my phone. My actual phone resolution is just a bit bigger than 1440p.

Thanks

@jobthomas
Copy link
Contributor Author

I have a 1920x1080 monitor at home and work for my phone. My actual phone resolution is just a bit bigger than 1440p.

Nothing to worry then. It's only for devices smaller than that breakpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Mobile) :hover styles on component menus are following thumb movements
4 participants